Skip to content

[V0][V1][Core] Add outlines integration for V1, and update V0 integration. #15975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

[V0][V1][Core] Add outlines integration for V1, and update V0 integration. #15975

wants to merge 16 commits into from
+602 −276

Conversation

unaidedelf8777
Copy link

@unaidedelf8777 unaidedelf8777 commented Apr 3, 2025 •
edited by github-actions bot
Loading

Adds outlines as a guided decoding backend for V1, and updates the integration for V0.

The aim of this is three fold:

  1. Remove the dependency on outlines, and only use outlines_core
  2. performance gains for V0 using the write_mask_into method on Guide to write a bitmask in-place for use in logits masking.
  3. outlines backend for V1

Because the dependency on outlines will be removed, support for grammar based decoding with the outlines backend will also be removed (CFG classes reside in the outlines package)

cc @aarnphm

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@unaidedelf8777 unaidedelf8777 changed the title [V0][V1][Core] Add outlines integration for V1, and update V0 integration. [V0][V1][Core] Add outlines integration for V1, and update V0 integration. [DO NOT MERGE] Apr 3, 2025
@mergify mergify bot added the v1 label Apr 3, 2025
@unaidedelf8777 unaidedelf8777 marked this pull request as draft April 3, 2025 00:46
@unaidedelf8777 unaidedelf8777 changed the title [V0][V1][Core] Add outlines integration for V1, and update V0 integration. [DO NOT MERGE] [V0][V1][Core] Add outlines integration for V1, and update V0 integration. Apr 3, 2025
@unaidedelf8777
Copy link
Author

NOTE: Can't be merged until next version of outlines_core is released.

@hmellor hmellor mentioned this pull request Apr 3, 2025
Closed
@russellb
Copy link
Member

russellb commented Apr 7, 2025

Thank you for the PR! I will review it this week.

aarnphm
aarnphm approved these changes Apr 7, 2025
View reviewed changes
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the v0 code path. One ask is to add tests for this for disabling cache path.

And we should update the requirements/common.txt to the lowest version of outlines-core supported.

@mergify mergify bot added tpu Related to Google TPUs ci/build and removed tpu Related to Google TPUs labels Apr 9, 2025
@mergify Mergify
Copy link

mergify bot commented Apr 11, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @unaidedelf8777.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@unaidedelf8777
Copy link
Author

@russellb @aarnphm All code is done. If you guys approve of it I’ll go ahead and clean up all the linter complaints, and then it should be ready.

Also outlines-core update has been pushed to pypi and pinned here (v0.2.9)

@unaidedelf8777 unaidedelf8777 marked this pull request as ready for review April 13, 2025 17:55
aarnphm
aarnphm suggested changes Apr 14, 2025
View reviewed changes
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of review. A few things needs to be addressed here. but great progress so far.

@mergify Mergify
Copy link

mergify bot commented Apr 18, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @unaidedelf8777.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@unaidedelf8777
pin newest outlines version
fd1a994 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
fix interface to be compliant with vllm-project#16577
5388bdc 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
make pre-commit happy
865d8a9 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
artifact from a prior merge
309c968 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
fix failing test
126328c 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
make pre-commit happy again
8022c7a 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777 unaidedelf8777 force-pushed the update-outlines-integration branch from db324d3 to 8022c7a Compare May 15, 2025 22:10
@mergify mergify bot removed the needs-rebase label May 15, 2025
@unaidedelf8777
Copy link
Author

unaidedelf8777 commented May 16, 2025 •
edited
Loading

Merge conflicts with your last PR are resolved. The test failure also seems to be unrelated to any changes in this PR.

@simon-mo simon-mo added the ready ONLY add when PR is ready to merge/full CI is needed label May 16, 2025
@simon-mo simon-mo enabled auto-merge (squash) May 16, 2025 05:30
@aarnphm
Copy link
Collaborator

aarnphm commented May 16, 2025

hmm maybe tried to merge from main ?

@aarnphm aarnphm mentioned this pull request May 17, 2025
Open
@aarnphm aarnphm removed this from the v0.9.0 milestone May 17, 2025
@aarnphm
Copy link
Collaborator

aarnphm commented May 17, 2025

@unaidedelf8777 I think only the logit processor tests in v0 is related to this PR here (at least for the outlines case)

PTAL

auto-merge was automatically disabled May 17, 2025 01:52

Head branch was pushed to by a user without write access

@unaidedelf8777
fix test_guided_json_object
3d4c13d 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777 unaidedelf8777 force-pushed the update-outlines-integration branch from 64c0086 to 3d4c13d Compare May 17, 2025 01:52
@unaidedelf8777
Fix test_guided_processors to be compatible with new integration
6ad15dc 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
pre-commit
53be045 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777
fix
05023f6 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777 unaidedelf8777 force-pushed the update-outlines-integration branch from e70ee76 to 05023f6 Compare May 17, 2025 17:42
@unaidedelf8777
fix tests
8f12c49 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@russellb
Copy link
Member

I would try merging from main. One of the test failures I see should be fixed.

@mergify mergify bot added the tool-calling label May 19, 2025
@unaidedelf8777
fix tool-use-tests build_regex_from_schema import path
d30598d 
Signed-off-by: Nathan Hoos <thwackyy.y@gmail.com>
@unaidedelf8777 unaidedelf8777 force-pushed the update-outlines-integration branch from 1fd1223 to d30598d Compare May 19, 2025 16:15
@unaidedelf8777
Copy link
Author

unaidedelf8777 commented May 19, 2025 •
edited
Loading

Just pushed a import path fix for the tool-use tests, so that should be happy now. The other failing tests seem unrelated to any changes here AFAICT.

@unaidedelf8777
Copy link
Author

@aarnphm Just because I didn't mention it in the last message, all of the V0 logits processors tests are green now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarnphm aarnphm aarnphm approved these changes

@mgoin mgoin Awaiting requested review from mgoin mgoin is a code owner

@russellb russellb Awaiting requested review from russellb russellb is a code owner

@WoosukKwon WoosukKwon Awaiting requested review from WoosukKwon WoosukKwon is a code owner

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

@njhill njhill Awaiting requested review from njhill njhill is a code owner

@ywang96 ywang96 Awaiting requested review from ywang96 ywang96 is a code owner

@comaniac comaniac Awaiting requested review from comaniac comaniac is a code owner

@alexm-redhat alexm-redhat Awaiting requested review from alexm-redhat alexm-redhat is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@russellb russellb

@aarnphm aarnphm

Labels
ci/build ready ONLY add when PR is ready to merge/full CI is needed structured-output tool-calling v1
Projects
Structured Output
Status: In review
Tool Calling
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@unaidedelf8777 @russellb @aarnphm @simon-mo